Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WD-11261] Implementation of LXD PWA with a dynamic start_url #785

Merged
merged 1 commit into from
Jun 5, 2024
Merged

[WD-11261] Implementation of LXD PWA with a dynamic start_url #785

merged 1 commit into from
Jun 5, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented May 28, 2024 •
edited
Loading

Done

  • Implemented minimal PWA support for the LXD-UI client in index.html.
    • The manifest file/variable supports a dynamic start_url link.
  • Added a Logo image for the minimum requirements of the PWA.
  • Added a LXD-Screenshot image for the minimum requirements of the PWA.

Specification: LX078 - PWA Support

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Open the LXD-UI Client and note the PWA pop up, or, view the
      image
      --icon in the url toolbar. One can click this to trigger the installation sequence.

Screenshots

image
image
image

@Kxiru Kxiru requested review from edlerd and mas-who May 28, 2024 14:00
@Kxiru Kxiru self-assigned this May 28, 2024
@webteam-app
Copy link

Demo

Jenkins

demos.haus

@Kxiru Kxiru marked this pull request as draft May 28, 2024 14:01
edlerd
edlerd reviewed May 28, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this up!

Some comments below to make it work on the demo server.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@mas-who
Copy link
Collaborator

mas-who commented May 30, 2024 •
edited
Loading

@Kxiru I think David has covered all the points. After making the changes for the screenshot reference path, it should work on the demo server as well.

In case if you are wondering why the path should be changed, you can go to the Source tab in your chrome inspector (see below for reference)

Screenshot from 2024-05-30 09-57-49

Edit: Can you also ask @piperdeck to do a test on Safari for this.

edlerd
edlerd requested changes Jun 4, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paths to the assets need to be prefixed with /ui and the scope field needs fixing.

I am not sure the orange menu bar is the best option, it seems a rather busy colour for it. Maybe use the grey/black from the left-hand navigation, or consult with design for this.

index.html Outdated Show resolved Hide resolved
edlerd
edlerd requested changes Jun 4, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works on localhost, but not yet on the demo server -- and I fear it won't work in the real build either. See comments below.

index.html Outdated Show resolved Hide resolved
manifest.js Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@Kxiru Kxiru force-pushed the PWA-support branch 3 times, most recently from 635dd11 to fbe04bd Compare June 4, 2024 21:24
@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 4, 2024

@edlerd , I've got it to work on the demo server and I've tidied up index.html a bit too :).

@Kxiru Kxiru requested a review from edlerd June 4, 2024 21:29
edlerd
edlerd reviewed Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great on the demo server with signed certificates!
I found some small improvements that should be done before merging it.

public/assets/js/manifest.js Outdated Show resolved Hide resolved
public/assets/js/manifest.js Outdated Show resolved Hide resolved
public/assets/img/LXD-screenshot2.png Outdated Show resolved Hide resolved
public/assets/js/manifest.js Outdated Show resolved Hide resolved
@Kxiru
Implementation of LXD PWA with a dynamic start_url"
ce06a56 
- Added Screenshot images to /public/assets/img/ for use in teh manifest variable in index.html.

Signed-off-by: Nkeiruka <[email protected]>
@Kxiru Kxiru requested a review from edlerd June 5, 2024 14:09
edlerd
edlerd approved these changes Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for driving this forward.

@Kxiru Kxiru marked this pull request as ready for review June 5, 2024 15:49
@Kxiru Kxiru merged commit 0e5e934 into canonical:main Jun 5, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 5, 2024
workflow: remove all reports for PR #785
cce778b
@Kxiru Kxiru deleted the PWA-support branch June 17, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edlerd edlerd edlerd approved these changes

@mas-who mas-who Awaiting requested review from mas-who

Assignees

@Kxiru Kxiru

Labels
Review: Code +1 Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kxiru @webteam-app @mas-who @edlerd